-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Code Review #1
base: review-1-target
Are you sure you want to change the base?
Code Review #1
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the submission, we will review the comments together.
background-color: #f3f3f3; | ||
} | ||
|
||
#info{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid using IDs for selectors.
@@ -0,0 +1,215 @@ | |||
@import url('https://fonts.googleapis.com/css?family=Courgette|Roboto'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use link
instead of import
to enable parallel downloads.
border:0; | ||
padding-left: 5px; | ||
border-radius:3px 0 0 3px; | ||
outline:none; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Outlines should only be modified using :focus.
margin-left:10px; | ||
} | ||
|
||
section h2{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heading (h2) should not be qualified.
} | ||
|
||
function showUsersTable(){ | ||
httpRequest = new XMLHttpRequest(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add the "let", "const" or "var" keyword to this declaration of "httpRequest" to make it explicit.
thead.appendChild(trHead); | ||
} | ||
|
||
var tr = document.createElement("tr"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename "tr" as this name is already used in declaration at line 89.
tr.appendChild(tdNome); | ||
tr.appendChild(tdEmail); | ||
tr.dataset.id = data[i].id; | ||
tr.onclick = function(){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Define this function outside of a loop.
} | ||
} | ||
|
||
function mountTablePublication(data){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function has very similar construct as mountTableUsers
and many duplicated lines, consider refactoring both into a base function.
} | ||
|
||
function objectToHTML(data, property){ | ||
for(a in data){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add the "let", "const" or "var" keyword to this declaration of "a" to make it explicit.
<body> | ||
<header class="flex"> | ||
<figure class="logo flex vCenter center row"> | ||
<img src="./img/logo.svg"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add an "alt" attribute to all images.
No description provided.